fix: correctly set track_latest on MMR
#1633
Conversation
igamigo
force-pushed
the
igamigo-fix-debug-assertion
branch
from
76258ae to
95b7ab5
Compare
igamigo
force-pushed
the
igamigo-fix-debug-assertion
branch
from
5e6cd95 to
329ea51
Compare
igamigo
force-pushed
the
igamigo-fix-debug-assertion
branch
from
329ea51 to
077be9f
Compare
bobbinth
left a comment
bobbinth
left a comment
There was a problem hiding this comment.
Looks good! Thank you! I left a couple of small comments inline - but other than that, this should be good to merge.
| let expected_depth = forest | ||
| .leaf_to_corresponding_tree(block_num.as_usize()) | ||
| .expect("forest includes block number") as usize; |
There was a problem hiding this comment.
nit: technically what we get here is expected_path_len - expected depth would be expected_path_len + 1.
Also, I think the assert! on lines 104 - 107 is redundant with the expect() here. I think we can probably remove the assert!.
| for node in merkle_path.nodes().iter().take(expected_depth) { | ||
| let sibling = idx.sibling(); | ||
| // NOTE: For a leaf that's in a smaller forest, all siblings within the tree depth should be | ||
| // in-bounds so this check and break are mostly redundant/should never be hit | ||
| // (iterating up to `expected_depth` handles it) | ||
| if sibling > rightmost_index { | ||
| break; | ||
| } | ||
| path_nodes.push((sibling, *node)); | ||
| idx = idx.parent(); | ||
| } |
There was a problem hiding this comment.
As the comment says here, the sibling check is redundant. I'd probably just simplify this to be:
for node in merkle_path.nodes().iter().take(expected_depth) {
path_nodes.push((idx.sibling(), *node));
idx = idx.parent();
}If we do want to be extra cautious, I'd convert the check into an assert!.
| cargo nextest run --workspace $(EXCLUDE_WASM_PACKAGES) --exclude testing-remote-prover --release --test=integration --run-ignored ignored-only -- import_genesis_accounts_can_be_used_for_transactions | ||
|
|
||
| .PHONY: test-dev | ||
| test-dev: ## Run tests with debug assertions enabled via test-dev profile |
There was a problem hiding this comment.
I think we should also change the CI job to try and catch if debug assertions fail
This PR introduces 2 fixes:
track_latestonly based on whether there were client notes included in the block. This was not sufficient astrack_latestis only meaningful if the leaf forms a single-leaf tree; in other situations the MMR code expects that the leaf will already be merged otherwiseIt also adds a couple make commands to run tests with
test-dev(introduced in #1400) to run tests with debug assertionsWe should probably look into adding this to
mainas well